fix: transactions paging using effective order #972
Conversation
WalkthroughThe changes introduce a mechanism in the ledger's paginator to convert pagination IDs to the correct SQL types based on table schema metadata. This is achieved by adding a Changes
Sequence Diagram(s)sequenceDiagram
participant Client
participant API
participant Store
participant Paginator
participant Schema
Client->>API: Request transactions with pagination
API->>Store: Fetch transactions (with paginator)
Store->>Paginator: Initialize with Table metadata
Paginator->>Schema: Lookup column SQL type
Paginator->>Paginator: Convert PaginationID to SQL type
Paginator->>Store: Return paginated query
Store->>API: Return transactions
API->>Client: Respond with paginated result
Possibly related PRs
Suggested reviewers
Poem
✨ Finishing Touches
🧪 Generate Unit Tests
Thanks for using CodeRabbit! It's free for OSS, and your support helps us grow. If you like it, consider giving us a shout-out. 🪧 TipsChatThere are 3 ways to chat with CodeRabbit:
SupportNeed help? Create a ticket on our support page for assistance with any issues or questions. Note: Be mindful of the bot's finite context window. It's strongly recommended to break down tasks such as reading entire modules into smaller chunks. For a focused discussion, use review comments to chat about specific files and their changes, instead of using the PR comments. CodeRabbit Commands (Invoked using PR comments)
Other keywords and placeholders
Documentation and Community
|
![coderabbitai[bot]](https://avatars.githubusercontent.com/in/347564?s=60&v=4){kind=small}
There was a problem hiding this comment.
Actionable comments posted: 3
🧹 Nitpick comments (1)
internal/storage/ledger/paginator_column.go (1)
73-81: Handle all timestamp variants inconvertPaginationIDToSQLTypePostgreSQL reports timestamp columns as
"timestamp without time zone"and"timestamp with time zone"(AKAtimestamptz).
Currently only the first variant is handled, so paginating on atimestamptzcolumn will silently fall back to*big.Int, leading to mismatched types in the SQL driver.-case "timestamp without time zone", "timestamp": +case "timestamp without time zone", "timestamp", "timestamp with time zone", "timestamptz":(Optional) cover
date,time, and numeric types explicitly for completeness.
📜 Review details
Configuration used: .coderabbit.yaml
Review profile: CHILL
Plan: Pro
📒 Files selected for processing (3)
internal/storage/ledger/paginator_column.go(4 hunks)internal/storage/ledger/store.go(2 hunks)test/e2e/api_transactions_list_test.go(1 hunks)
🧰 Additional context used
🧬 Code Graph Analysis (1)
test/e2e/api_transactions_list_test.go (2)
pkg/testserver/api.go (1)
ListTransactions(139-147)pkg/client/models/operations/v2listtransactions.go (1)
V2ListTransactionsRequest(36-53)
⏰ Context from checks skipped due to timeout of 90000ms (2)
- GitHub Check: Dirty
- GitHub Check: Tests
🔇 Additional comments (1)
test/e2e/api_transactions_list_test.go (1)
222-237: Good addition – validates cursor paging with effective orderingThe new “using next page” context exercises the exact scenario that broke previously.
It keeps the request minimal (onlycursor), ensuring the server picks the page size from the encoded cursor, and theHaveLen(pageSize)assertion makes that explicit.
Nice coverage.
| return newPaginatedResourceRepository(store, store.ledger, &transactionsResourceHandler{}, columnPaginator[ledger.Transaction, any]{ | ||
| defaultPaginationColumn: "id", | ||
| defaultOrder: bunpaginate.OrderDesc, | ||
| Table: store.db.Dialect().Tables().ByName("transactions"), | ||
| }) | ||
| } |
There was a problem hiding this comment.
Guard against nil table metadata to avoid panics at runtime
store.db.Dialect().Tables().ByName("transactions") (and "logs") can legally return nil if the schema introspection misses the table name (e.g. because of a non-default schema, quoting, or a typo).
columnPaginator.paginate() immediately dereferences o.Table.FieldMap[...], which would panic in that situation (or when a caller specifies a Column that is not present in the FieldMap).
Consider resolving the table upfront and failing fast (or falling back) instead of letting the paginator crash later:
- Table: store.db.Dialect().Tables().ByName("transactions"),
+ tbl := store.db.Dialect().Tables().ByName("transactions")
+ if tbl == nil {
+ // You may want to log and bubble up a coherent error instead of panicking
+ panic("transactions table metadata not found – paginator needs schema information")
+ }
+ Table: tbl,Do the same for the "logs" paginator or, even better, extend columnPaginator.paginate() to tolerate a missing Table / column and fall back to the raw big.Int value.
Fail-fast here prevents production panics and makes the error surface early in the boot path.
Also applies to: 76-81
🤖 Prompt for AI Agents
In internal/storage/ledger/store.go around lines 65 to 70 and 76 to 81, the
calls to store.db.Dialect().Tables().ByName("transactions") and ByName("logs")
can return nil if the table metadata is missing, which causes panics later when
the paginator dereferences the table fields. To fix this, add checks immediately
after retrieving the table metadata to verify it is not nil. If nil, return an
error or handle the fallback gracefully to fail fast during initialization
instead of panicking at runtime. This ensures safer handling of missing table
metadata and prevents crashes.
| defaultPaginationColumn string | ||
| defaultOrder bunpaginate.Order | ||
| Table *schema.Table | ||
| } |
There was a problem hiding this comment.
🛠️ Refactor suggestion
Table should be nullable-safe or mandatory – pick one
You added the Table *schema.Table handle but client code can still instantiate columnPaginator{} without setting it (e.g., in future resources).
Because paginate() dereferences o.Table unconditionally, any forgotten assignment will panic.
Either:
- Make the field non-pointer (
schema.Table) and always populate it, or - Keep it a pointer but add defensive checks in
paginate()(see next comment).
🤖 Prompt for AI Agents
In internal/storage/ledger/paginator_column.go around lines 17 to 20, the Table
field is a pointer but can be nil, causing a panic when dereferenced in
paginate(). To fix this, either change Table to a non-pointer type and ensure it
is always set when creating columnPaginator instances, or keep it as a pointer
but add nil checks in paginate() to handle the case when Table is not assigned,
preventing runtime panics.
| paginationID := convertPaginationIDToSQLType( | ||
| o.Table.FieldMap[paginationColumn].DiscoveredSQLType, | ||
| query.PaginationID, | ||
| ) | ||
| if query.Reverse { | ||
| switch originalOrder { | ||
| case bunpaginate.OrderAsc: | ||
| sb = sb.Where(fmt.Sprintf("%s < ?", paginationColumn), query.PaginationID) | ||
| sb = sb.Where(fmt.Sprintf("%s < ?", paginationColumn), paginationID) | ||
| case bunpaginate.OrderDesc: | ||
| sb = sb.Where(fmt.Sprintf("%s > ?", paginationColumn), query.PaginationID) | ||
| sb = sb.Where(fmt.Sprintf("%s > ?", paginationColumn), paginationID) | ||
| } | ||
| } else { | ||
| switch originalOrder { | ||
| case bunpaginate.OrderAsc: | ||
| sb = sb.Where(fmt.Sprintf("%s >= ?", paginationColumn), query.PaginationID) | ||
| sb = sb.Where(fmt.Sprintf("%s >= ?", paginationColumn), paginationID) | ||
| case bunpaginate.OrderDesc: | ||
| sb = sb.Where(fmt.Sprintf("%s <= ?", paginationColumn), query.PaginationID) | ||
| sb = sb.Where(fmt.Sprintf("%s <= ?", paginationColumn), paginationID) | ||
| } |
There was a problem hiding this comment.
Dereferencing FieldMap without checks is unsafe
o.Table.FieldMap[paginationColumn] will panic when:
o.Tableisnil.paginationColumnis not present in the table (e.g., user passed a computed column such aseffective).
A minimal, crash-free fallback:
- paginationID := convertPaginationIDToSQLType(
- o.Table.FieldMap[paginationColumn].DiscoveredSQLType,
- query.PaginationID,
- )
+ var sqlType string
+ if o.Table != nil {
+ if field, ok := o.Table.FieldMap[paginationColumn]; ok {
+ sqlType = field.DiscoveredSQLType
+ }
+ }
+ paginationID := convertPaginationIDToSQLType(sqlType, query.PaginationID)This still honours type conversion when metadata exists but gracefully degrades to the previous behaviour otherwise.
📝 Committable suggestion
‼️ IMPORTANT
Carefully review the code before committing. Ensure that it accurately replaces the highlighted code, contains no missing lines, and has no issues with indentation. Thoroughly test & benchmark the code to ensure it meets the requirements.
| paginationID := convertPaginationIDToSQLType( | |
| o.Table.FieldMap[paginationColumn].DiscoveredSQLType, | |
| query.PaginationID, | |
| ) | |
| if query.Reverse { | |
| switch originalOrder { | |
| case bunpaginate.OrderAsc: | |
| sb = sb.Where(fmt.Sprintf("%s < ?", paginationColumn), query.PaginationID) | |
| sb = sb.Where(fmt.Sprintf("%s < ?", paginationColumn), paginationID) | |
| case bunpaginate.OrderDesc: | |
| sb = sb.Where(fmt.Sprintf("%s > ?", paginationColumn), query.PaginationID) | |
| sb = sb.Where(fmt.Sprintf("%s > ?", paginationColumn), paginationID) | |
| } | |
| } else { | |
| switch originalOrder { | |
| case bunpaginate.OrderAsc: | |
| sb = sb.Where(fmt.Sprintf("%s >= ?", paginationColumn), query.PaginationID) | |
| sb = sb.Where(fmt.Sprintf("%s >= ?", paginationColumn), paginationID) | |
| case bunpaginate.OrderDesc: | |
| sb = sb.Where(fmt.Sprintf("%s <= ?", paginationColumn), query.PaginationID) | |
| sb = sb.Where(fmt.Sprintf("%s <= ?", paginationColumn), paginationID) | |
| } | |
| // Safely derive the SQL type for pagination, falling back if metadata is missing. | |
| var sqlType string | |
| if o.Table != nil { | |
| if field, ok := o.Table.FieldMap[paginationColumn]; ok { | |
| sqlType = field.DiscoveredSQLType | |
| } | |
| } | |
| paginationID := convertPaginationIDToSQLType(sqlType, query.PaginationID) | |
| if query.Reverse { | |
| switch originalOrder { | |
| case bunpaginate.OrderAsc: | |
| sb = sb.Where(fmt.Sprintf("%s < ?", paginationColumn), paginationID) | |
| case bunpaginate.OrderDesc: | |
| sb = sb.Where(fmt.Sprintf("%s > ?", paginationColumn), paginationID) | |
| } | |
| } else { | |
| switch originalOrder { | |
| case bunpaginate.OrderAsc: | |
| sb = sb.Where(fmt.Sprintf("%s >= ?", paginationColumn), paginationID) | |
| case bunpaginate.OrderDesc: | |
| sb = sb.Where(fmt.Sprintf("%s <= ?", paginationColumn), paginationID) | |
| } | |
| } |
🤖 Prompt for AI Agents
In internal/storage/ledger/paginator_column.go around lines 49 to 66, the code
dereferences o.Table.FieldMap[paginationColumn] without checking if o.Table is
nil or if paginationColumn exists in FieldMap, which can cause a panic. Fix this
by adding checks to ensure o.Table is not nil and paginationColumn is present in
FieldMap before accessing it. If these conditions are not met, fall back to the
previous behavior without type conversion to avoid crashes while still honoring
type conversion when metadata exists.
There was a problem hiding this comment.
Actionable comments posted: 0
🧹 Nitpick comments (2)
internal/storage/driver/module.go (2)
5-5: Import aliasledgeris easily confused withledgerstoreYou now have two imports whose aliases differ only by the suffix
store. Although legal, this can slow down review/reading because the two names are visually similar but point to very different packages. Consider a more explicit alias (e.g.coreledgerordomain) for one of them to reduce cognitive load.
25-33: Register models viadb.RegisterModeland use nil-pointers to avoid needless allocations
db.Dialect().Tables().Register(&T{})works, but:
bun.DBexposesRegisterModel(...)which is shorter and future-proof across dialect implementations.- Passing an address-of value allocates five struct instances every time the Fx graph is built. Using typed nil pointers avoids that cost while retaining type information (
(*ledger.Transaction)(nil)).Example diff:
- fx.Invoke(func(db *bun.DB) { - db.Dialect().Tables().Register( - &ledger.Transaction{}, - &ledger.Log{}, - &ledger.Account{}, - &ledger.Move{}, - &ledger.Ledger{}, - ) - }), + fx.Invoke(func(db *bun.DB) { + db.RegisterModel( + (*ledger.Transaction)(nil), + (*ledger.Log)(nil), + (*ledger.Account)(nil), + (*ledger.Move)(nil), + (*ledger.Ledger)(nil), + ) + }),This keeps the intent intact, shaves a tiny allocation, and leans on the higher-level helper Bun already provides.
📜 Review details
Configuration used: .coderabbit.yaml
Review profile: CHILL
Plan: Pro
📒 Files selected for processing (2)
internal/storage/driver/module.go(2 hunks)internal/storage/ledger/resource.go(0 hunks)
💤 Files with no reviewable changes (1)
- internal/storage/ledger/resource.go
🧰 Additional context used
🧬 Code Graph Analysis (1)
internal/storage/driver/module.go (3)
internal/storage/ledger/logs.go (1)
Log(20-26)internal/account.go (1)
Account(14-24)internal/moves.go (1)
Move(12-24)
⏰ Context from checks skipped due to timeout of 90000ms (1)
- GitHub Check: Tests
No description provided.